Fix user API key migration failing on populated databases - #90
Open
turegjorup wants to merge 2 commits into
Open
Fix user API key migration failing on populated databases#90turegjorup wants to merge 2 commits into
turegjorup wants to merge 2 commits into
Conversation
The existing Doctrine jobs migrate an empty database, so a migration that cannot cope with existing rows passes them unnoticed. Build the database as it looks before the pull request, fixtures included, then apply the pull request's migrations on top. Fixtures created a single user, and one row cannot collide with itself, so add a few more.
Version20260702123347 added user.api_key as NOT NULL and then put a unique index on it, so every existing user got the same empty string and the index collided on the second row. Any database holding more than one user failed to migrate. Generate a key for each existing user before creating the index, matching App\Entity\User::__construct, which gives every user created since #88 a key of its own. The keys come from PHP rather than SQL because they are credentials and SQL's RAND() and UUID() are not cryptographically secure.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #90 +/- ##
=============================================
+ Coverage 37.14% 37.63% +0.49%
- Complexity 948 955 +7
=============================================
Files 133 135 +2
Lines 2972 2997 +25
=============================================
+ Hits 1104 1128 +24
- Misses 1868 1869 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to ticket
https://leantime.itkdev.dk/_#/tickets/showTicket/7910
Description
Version20260702123347from #88 addsuser.api_keyasNOT NULLand then indexes it uniquely, so every existing user gets the same empty string and the migration fails on the second row. Staging and production have not run it yet, but both would fail on the next release.It now generates a key per existing user first, matching
User::__construct, so existing users gain a dormant API key. Keys come from PHP because SQL'sRAND()andUUID()are not cryptographically secure.The new CI job migrates a populated database, unlike the existing jobs. It cannot go red on this bug, whose migration is already in this PR's base, so it is forward protection only.
Screenshot of the result
No user interface changes.
Checklist
Additional comments or questions
Existing users gain a working API key, which is the one decision here a follow-up migration cannot undo: afterwards the keys cannot be told apart from ones the application generated. The alternative was a nullable column, dropped because
User::__constructalready gives every new user a key.